Skip to content

VE.Direct frame error logging#1810

Open
SW-Niko wants to merge 5 commits intohoylabs:developmentfrom
SW-Niko:VeFrameErrorLog
Open

VE.Direct frame error logging#1810
SW-Niko wants to merge 5 commits intohoylabs:developmentfrom
SW-Niko:VeFrameErrorLog

Conversation

@SW-Niko
Copy link
Copy Markdown

@SW-Niko SW-Niko commented Apr 4, 2025

I've implemented a transmission error log for the VE.Direct connections (charge controller, shunt). The Live View displays the recorded transmission errors for each device per day. This allows you to check the quality of your VE.Direct connection.

grafik

Additionally, the log displays transmission errors in more detail every 60 seconds. You can see which errors (checksum, buffer overflow, etc.) were detected.

23:13:09.079 > [VE.Direct MPPT 10/9] Average transmission errors per day: 0.0
23:13:09.079 > [VE.Direct MPPT 10/9] Total Sum: 0, Frame Timeout: 0, Text Checksum: 0
23:13:09.133 > [VE.Direct MPPT 10/9] Hex Checksum: 0, Hex Buffer: 0, Nested Hex: 0
23:13:09.133 > [VE.Direct MPPT 10/9] Debug Buffer: 0, Unknown Data: 0, Invalid Char: 0

For the sake of completeness, I should also mention:

  • We cannot detect all errors (multiple errors with the correct checksum).
  • Sometimes a transmission error triggers multiple individual errors.
  • No average is calculated on day 1.
  • When the total error reaches 50,000, all error counters are automatically reset.

@SW-Niko SW-Niko marked this pull request as draft April 4, 2025 06:54
@AndreasBoehm AndreasBoehm requested a review from Copilot April 4, 2025 07:30
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 7 out of 10 changed files in this pull request and generated 1 comment.

Files not reviewed (3)
  • webapp/src/locales/de.json: Language not supported
  • webapp/src/locales/en.json: Language not supported
  • webapp/src/locales/fr.json: Language not supported

Comment thread lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp Outdated
@SW-Niko
Copy link
Copy Markdown
Author

SW-Niko commented Apr 10, 2025

Update, System 2 days and 12h online

Live View:
grafik

Logging:
07:10:58.475 > [VE.Direct MPPT 11/12] Average transmission errors per day: 1.6
07:10:58.475 > [VE.Direct MPPT 11/12] Total Sum: 4, Frame Timeout: 0, Text Checksum: 3
07:10:58.569 > [VE.Direct MPPT 11/12] Hex Checksum: 0, Hex Buffer: 0, Nested Hex: 0
07:10:58.569 > [VE.Direct MPPT 11/12] Debug Buffer: 0, Unknown Data: 0, Invalid Char: 1

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 1, 2025

Build Artifacts

Firmware built from this pull request's code:

Notice

  • These artifacts are ZIP files containing the factory update binary as well as the OTA update binary.
    Extract the binaries from the ZIP files first. Do not use the ZIP files themselves to perform an update.
  • These links point to artifacts of the latest successful build run.
  • The linked artifacts were built from 84bdb68.

@SW-Niko SW-Niko marked this pull request as ready for review May 1, 2025 08:14
@AndreasBoehm AndreasBoehm requested a review from Copilot May 1, 2025 18:46
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds functionality to log and display VE.Direct transmission errors in greater detail for both the MPPT charge controllers and the Victron SmartShunt. Key changes include:

  • Adding transmission error values in the JSON output for MPPT and SmartShunt.
  • Introducing new error counter and logging functions in the VeDirect frame handler.
  • Updating the VE.Direct data structure to map error codes to human‐readable strings.

Reviewed Changes

Copilot reviewed 8 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/solarcharger/victron/Stats.cpp Adds JSON entries for MPPT transmission errors.
src/battery/victronsmartshunt/Stats.cpp Updates Live View data to include transmission error value.
lib/VeDirectFrameHandler/VeDirectFrameHexHandler.cpp Updates error handling to include HEX_CHECKSUM errors.
lib/VeDirectFrameHandler/VeDirectFrameHandler.h Declares new error counter and logging function members.
lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp Implements error counting and logging, including transmission error averaging.
lib/VeDirectFrameHandler/VeDirectData.h and VeDirectData.cpp Adds mapping for transmission error codes to readable strings.
include/battery/victronsmartshunt/Stats.h Adds a new member to hold transmission error data for the SmartShunt.
Files not reviewed (3)
  • webapp/src/locales/de.json: Language not supported
  • webapp/src/locales/en.json: Language not supported
  • webapp/src/locales/fr.json: Language not supported
Comments suppressed due to low confidence (1)

src/solarcharger/victron/Stats.cpp:255

  • [nitpick] Consider aligning the naming for transmission error keys across device types. For consistency, either use 'MpptTransmitError' or a similar naming convention as used in the SmartShunt ('transmitError').
device["MpptTransmitError"]["v"] = mpptData.transmissionErrors_Day;

Comment thread lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp Outdated
@SW-Niko
Copy link
Copy Markdown
Author

SW-Niko commented May 30, 2025

Now using DTU_LOGI to log the transmission errors.

@SW-Niko
Copy link
Copy Markdown
Author

SW-Niko commented Jun 13, 2025

Small improvement: Do not print the transmission faults in the logging if we do not have any.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 30, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e83c677c-e0f8-4f9b-8777-2b043d7e8502

📥 Commits

Reviewing files that changed from the base of the PR and between e079f5f and 7fd97ad.

📒 Files selected for processing (1)
  • lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp
✅ Files skipped from review due to trivial changes (1)
  • lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp

Walkthrough

Adds transmit-error tracking across VE-Direct parsing: new Error enum and per-error counters in the frame handler, periodic conversion to a daily rate, propagation into veStruct, battery/charger stats, and UI localization keys.

Changes

Cohort / File(s) Summary
Data structure additions
include/battery/victronsmartshunt/Stats.h, lib/VeDirectFrameHandler/VeDirectData.h
Added private float _transmitErrors to VictronSmartShunt::Stats; added float transmitErrors_Day, enum class Error { ... , LAST }, and getTransmitErrorAsString(Error) const to veStruct.
VE-Direct frame handling — core
lib/VeDirectFrameHandler/VeDirectFrameHandler.h, lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp
Added per-error counters array, cumulative startup sum, last-print timestamp, _prevState; new private helpers setErrorCounter(Error) and printErrorCounter(); integrated error increments across multiple parse-failure paths and periodic 60s daily-rate computation and conditional logging.
VE-Direct frame handling — hex parse
lib/VeDirectFrameHandler/VeDirectFrameHexHandler.cpp
Tightened hex disassembly checks: short frames now set HEX_BUFFER, checksum failures set HEX_CHECKSUM; parsing proceeds only when len > 3 and checksum OK; error counters updated on failures.
Error string mapping
lib/VeDirectFrameHandler/VeDirectData.cpp
Added implementation mapping veStruct::Error values to frozen::string labels via getTransmitErrorAsString.
Stats integration
src/battery/victronsmartshunt/Stats.cpp, src/solarcharger/victron/Stats.cpp
Stats now read transmitErrors_Day into _transmitErrors/JSON fields and expose it in live-view/JSON with unit 1/d.
Localization
webapp/src/locales/en.json, webapp/src/locales/de.json
Added translation keys solarchargerhome.device.MpptTransmitError and battery.transmitError (English: "Transmit errors", German: "Übertragungsfehler").

Sequence Diagram

sequenceDiagram
    participant Frame as FrameHandler
    participant Data as VeDirectData
    participant Stats as StatsModule
    participant UI as WebUI

    Frame->>Frame: receive bytes / parse
    Frame->>Frame: detect error (TIMEOUT, CHECKSUM, BUFFER, etc.)
    Frame->>Frame: call setErrorCounter(Error) -> increment per-type counter and _errorSumSinceStartup

    Note over Frame: every 60s
    Frame->>Frame: compute transmitErrors_Day from _errorSumSinceStartup
    Frame->>Data: assign transmitErrors_Day

    Data->>Stats: updateFrom(veStruct)
    Stats->>Stats: store _transmitErrors and add to live/JSON payloads
    Stats->>UI: publish localized payload
    UI->>UI: render label and value
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I nibbled bytes and chased each bug,

counted hiccups, checksum and shrug,
rolled them up to one neat score,
sent them out to show and store.
A tiny hop — the dashboard hums! 🥕

🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The PR description clearly describes the transmission error logging feature for VE.Direct connections, explaining the Live View display and detailed error logs.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
include/battery/victronsmartshunt/Stats.h (1)

34-34: Initialize the new member explicitly.

Please default-initialize _transmissionErrors to avoid undefined startup values in any non-zero-initialized construction path.

♻️ Proposed change
-    float _transmissionErrors;
+    float _transmissionErrors {0.0f};
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@include/battery/victronsmartshunt/Stats.h` at line 34, The new member float
_transmissionErrors is not explicitly initialized; initialize it to a known
value (e.g., 0.0f) to avoid undefined startup values by either giving it an
in-class default (float _transmissionErrors = 0.0f;) or adding it to the Stats
constructor initializer list (Stats(...) : ..., _transmissionErrors(0.0f) { }).
Target the _transmissionErrors field in the Stats class/constructor when making
this change.
lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp (2)

336-337: Consider whether UNKNOWN_TEXT_DATA should count as a transmission error.

Unknown fields may not indicate transmission errors but rather unsupported device features. Counting these as errors could inflate the transmission error metric for devices with extra fields that this code doesn't parse yet.

If this is intentional for monitoring purposes, consider documenting that this counter includes both actual errors and unsupported fields.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp` around lines 336 - 337,
The code currently treats unknown text fields as transmission errors by calling
setErrorCounter(veStruct::Error::UNKNOWN_TEXT_DATA) alongside DTU_LOGI in
VeDirectFrameHandler.cpp; change this so unsupported/unknown fields do not
inflate the transmission error metric — either remove the setErrorCounter call
for veStruct::Error::UNKNOWN_TEXT_DATA, replace it with a non-error counter
(e.g., a warning/unsupported-field metric), or gate the increment behind a
configurable flag, and add a brief comment documenting the chosen behavior so
future readers know whether UNKNOWN_TEXT_DATA is considered a true transmission
error or just an unsupported field.

122-134: Day 1 metric may be misleading to users.

When errorDays <= 1.0f, transmissionErrors_Day equals the raw error sum rather than a rate. Since the UI displays this with unit "1/d", users may interpret it as a daily rate when it's actually just the total count during the first day.

Consider either:

  1. Setting transmissionErrors_Day to 0 or a sentinel value until day 1 passes
  2. Computing the rate even on day 1 (e.g., errors / errorDays regardless of value)
  3. Documenting this behavior in the UI or logging
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp` around lines 122 - 134,
The current logic sets _tmpFrame.transmissionErrors_Day to the raw error sum
when errorDays <= 1.0f which mislabels a count as "1/d"; change the calculation
so transmissionErrors_Day is always a rate: compute errorDays =
esp_timer_get_time() / (24.0f*60*60*1000*1000) and set
_tmpFrame.transmissionErrors_Day =
_errorCounter.at(static_cast<size_t>(veStruct::Error::SUM)) / errorDays (handle
the unlikely zero case defensively), rather than skipping the division when
errorDays <= 1.0f; update any related logging/printing (printErrorCounter()
call) if you want to avoid noisy output on very small errorDays.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp`:
- Around line 392-408: The SUM-based halving in
VeDirectFrameHandler<T>::setErrorCounter (which updates _errorCounter and checks
veStruct::Error::SUM > 50000) skews transmissionErrors_Day because the numerator
is reduced but the elapsed-time denominator is not; fix by tracking extra state
(e.g., add a lastScaleTime or cumulativeScaleFactor member) and update how
transmissionErrors_Day is computed to account for the scale: either record the
timestamp when you halve counters and use that as the start time for rate
calculation, or maintain a cumulative scaling divisor applied when computing the
effective total so the daily rate reflects pre-scale counts; update
setErrorCounter to adjust the new state (lastScaleTime or cumulativeScaleFactor)
whenever you downscale so transmissionErrors_Day uses those values to compute an
accurate rate.

---

Nitpick comments:
In `@include/battery/victronsmartshunt/Stats.h`:
- Line 34: The new member float _transmissionErrors is not explicitly
initialized; initialize it to a known value (e.g., 0.0f) to avoid undefined
startup values by either giving it an in-class default (float
_transmissionErrors = 0.0f;) or adding it to the Stats constructor initializer
list (Stats(...) : ..., _transmissionErrors(0.0f) { }). Target the
_transmissionErrors field in the Stats class/constructor when making this
change.

In `@lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp`:
- Around line 336-337: The code currently treats unknown text fields as
transmission errors by calling
setErrorCounter(veStruct::Error::UNKNOWN_TEXT_DATA) alongside DTU_LOGI in
VeDirectFrameHandler.cpp; change this so unsupported/unknown fields do not
inflate the transmission error metric — either remove the setErrorCounter call
for veStruct::Error::UNKNOWN_TEXT_DATA, replace it with a non-error counter
(e.g., a warning/unsupported-field metric), or gate the increment behind a
configurable flag, and add a brief comment documenting the chosen behavior so
future readers know whether UNKNOWN_TEXT_DATA is considered a true transmission
error or just an unsupported field.
- Around line 122-134: The current logic sets _tmpFrame.transmissionErrors_Day
to the raw error sum when errorDays <= 1.0f which mislabels a count as "1/d";
change the calculation so transmissionErrors_Day is always a rate: compute
errorDays = esp_timer_get_time() / (24.0f*60*60*1000*1000) and set
_tmpFrame.transmissionErrors_Day =
_errorCounter.at(static_cast<size_t>(veStruct::Error::SUM)) / errorDays (handle
the unlikely zero case defensively), rather than skipping the division when
errorDays <= 1.0f; update any related logging/printing (printErrorCounter()
call) if you want to avoid noisy output on very small errorDays.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6e2ebcd5-87e6-4e9d-a8c4-b9857152eea3

📥 Commits

Reviewing files that changed from the base of the PR and between 5f8ccdd and 7a3f960.

📒 Files selected for processing (10)
  • include/battery/victronsmartshunt/Stats.h
  • lib/VeDirectFrameHandler/VeDirectData.cpp
  • lib/VeDirectFrameHandler/VeDirectData.h
  • lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp
  • lib/VeDirectFrameHandler/VeDirectFrameHandler.h
  • lib/VeDirectFrameHandler/VeDirectFrameHexHandler.cpp
  • src/battery/victronsmartshunt/Stats.cpp
  • src/solarcharger/victron/Stats.cpp
  • webapp/src/locales/de.json
  • webapp/src/locales/en.json

Comment thread lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/VeDirectFrameHandler/VeDirectData.cpp`:
- Around line 367-379: The code lacks a veStruct::Error value for the "Unknown
Data" bucket and so getTransmitErrorAsString cannot return a string for it; add
a new enum member (e.g., UNKNOWN_DATA) to veStruct::Error in the header and add
a corresponding entry in the frozen::map inside getTransmitErrorAsString (map
key UNKNOWN_DATA -> "Unknown Data"), then update any places that classify frames
as unknown-data to set that enum value so logs/UI can display it (refer to
getTransmitErrorAsString and the veStruct::Error enum).

In `@lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp`:
- Line 398: printErrorCounter() never emits the accumulated raw total stored in
_errorSumSinceStartup, so update the function(s) that format and log error
statistics (e.g., printErrorCounter and any related summary formatter used
around the block that handles entries at lines 414-431) to append a "Total Sum"
line showing _errorSumSinceStartup; specifically, locate printErrorCounter()
(and the summary code that formats _errorCounter entries) and add a formatted
output that includes the numeric value of _errorSumSinceStartup in the same log
output so the sample "Total Sum" line can be emitted.
- Around line 175-182: Initialize the member _prevState to a known safe value in
the constructor (e.g., State::IDLE) and change the nested-hex detection in
VeDirectFrameHandler:: (the block that currently checks if ((inbyte == ':') &&
(_state != State::CHECKSUM))) to test the current state (_state ==
State::RECORD_HEX) instead of _prevState when deciding to call
setErrorCounter(veStruct::Error::NESTED_HEX); keep the existing _prevState
update logic around the RECORD_HEX transition so the previous state is still
recorded for other uses.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 78ff47e8-0fdd-4977-b071-79be659dd57f

📥 Commits

Reviewing files that changed from the base of the PR and between 7a3f960 and e079f5f.

📒 Files selected for processing (10)
  • include/battery/victronsmartshunt/Stats.h
  • lib/VeDirectFrameHandler/VeDirectData.cpp
  • lib/VeDirectFrameHandler/VeDirectData.h
  • lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp
  • lib/VeDirectFrameHandler/VeDirectFrameHandler.h
  • lib/VeDirectFrameHandler/VeDirectFrameHexHandler.cpp
  • src/battery/victronsmartshunt/Stats.cpp
  • src/solarcharger/victron/Stats.cpp
  • webapp/src/locales/de.json
  • webapp/src/locales/en.json
✅ Files skipped from review due to trivial changes (2)
  • src/solarcharger/victron/Stats.cpp
  • src/battery/victronsmartshunt/Stats.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
  • include/battery/victronsmartshunt/Stats.h
  • webapp/src/locales/de.json
  • webapp/src/locales/en.json
  • lib/VeDirectFrameHandler/VeDirectFrameHexHandler.cpp
  • lib/VeDirectFrameHandler/VeDirectData.h
  • lib/VeDirectFrameHandler/VeDirectFrameHandler.h

Comment thread lib/VeDirectFrameHandler/VeDirectData.cpp
Comment thread lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp Outdated
Comment thread lib/VeDirectFrameHandler/VeDirectFrameHandler.cpp
SW-Niko added 5 commits April 21, 2026 17:51
- detect and count different transmit errors
- logging of errors
- can be used to qualify the transmit quality
- add error counter to  MPPT
- add error counter to  Smart Shunt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants